Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add total to TasksResult #412

Closed
wants to merge 2 commits into from
Closed

Conversation

Sherlouk
Copy link
Collaborator

Pull Request

Related issue

Fixes #402

What does this PR do?

  • Adds support for new total attribute to GET tasks request. This is a recent addition, and so it may be desired to make this optional for backwards compatibility.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @Sherlouk
Thank you for your PR

Looks like the CI is failing, can you fix the test?

 Test Case 'IndexesTests.testGetTasksWithParametersFromIndex' started at 2023-09-21 17:26:55.334
▿ Swift.DecodingError.keyNotFound
  ▿ keyNotFound: (2 elements)
    - .0: CodingKeys(stringValue: "total", intValue: nil)
    ▿ .1: Swift.DecodingError.Context
      - codingPath: 0 elements
      - debugDescription: "No value associated with key CodingKeys(stringValue: \"total\", intValue: nil) (\"total\")."
      - underlyingError: nil

Here is the CI so that you can see the full logs: https://github.com/meilisearch/meilisearch-swift/actions/runs/6213141814/job/17012982161?pr=412

@curquiza curquiza added the enhancement New feature or request label Sep 21, 2023
@Sherlouk
Copy link
Collaborator Author

Weird, let me take a look into that. It might be a version incompatibility issue, or an edge case where total isn't returned (spec says it's now required).

I'll look into it.

@Sherlouk
Copy link
Collaborator Author

Well I'm glad that it turned out that I was just being a bit silly and only ran the integration tests, as they're separate within Xcode! The unit tests have embedded JSON, not relying on the running of a local Meili instance.

I've updated, and run all tests this time, and am confident I've patched that now.

I've also familiarised myself with the GitHub Actions workflows so am more confident in what that does.

@Sherlouk Sherlouk force-pushed the 402-total-tasks branch 2 times, most recently from f73ce76 to 2067d70 Compare September 21, 2023 22:24
@Sherlouk
Copy link
Collaborator Author

Can see here that I was running into some random errors trying to download MeiliSearch itself - retrying resolved it (but explains the empty force commits).

@Sherlouk
Copy link
Collaborator Author

I'm going to close this PR as once #417 is merged I have a PR which covers the rest of task parity (and for the sake of 7 lines of code makes sense to include there).

@Sherlouk Sherlouk closed this Sep 23, 2023
meili-bors bot added a commit that referenced this pull request Oct 11, 2023
425: [BREAKING] Tasks Parity r=curquiza a=Sherlouk

Closes #363
Closes #366
Closes #368
Closes #254

## User/Client Facing

* Introduces Cancel Tasks API
* Introduces Delete Tasks API
* Provides compiler-safe access to task types
  * Utilising a future-proof solution avoiding errors should new task types be added without Swift support
* Provides compiler-safe access to task result details
  * Utilising a future-proof solution avoiding errors should new details be added without Swift support
* Grouped task related models into own directory
* Uses `Date` objects instead of `String` for dates within task models
* Added missing fields to existing task models
* Fixed error caused by global tasks
* Uses compiler-safe types in task query to improve safety
* Adds `total` to task results response (replacing #412)
* Removed all instances of `try!` (force try) to prevent crashes in test runner

## Breaking Changes

⚠️  This is a breaking change. Even with something as simple as moving to a enum for TaskType requires changes to clients. This PR looks to bring all task changes together meaning that clients can upgrade to this new and safe API design in one go.

* `Task.Status` has a new value `.canceled` (handle in any `switch` cases)
* `TaskType` replaces a previous String based API (use `.description` to access String)
* `TasksQuery` uses compiled types instead of String APIs (example: `"indexUpdate"` is now `.indexUpdate`)
* `enqueuedAt` now uses Date instead of ISO-8601 String

## Parity

Reviewing documentation available on MeiliSearch's website, this PR brings the `Tasks` API up to **100% parity** with existing features. It resolves many errors caused by the previous design (both architecturally but also instability in the current release), and provides future-proofness against further scope within the tasks API.

## Personal

As I am developing my own Swift based application for monitoring MeiliSearch (and it's tasks), this branch has been extremely helpful for me in exploring jobs and being able to cancel them and performing my own housekeeping.

The type safety has been delightful and actually detected a bug in my original implementation which had a typo in a task name.

Co-authored-by: James Sherlock <15193942+Sherlouk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.3] Total Tasks in task route
2 participants